Adding Vec support for transparent structs #199

rkreutz · 2023-03-16T22:50:02Z

Original PR was #193, but I had decided to open a PR from a feature branch rather than from master from my fork.

This PR addresses #91

Added all the shims so transparent structs can support Vec. A few caveats though:

Structs with swift_repr = "class" are not supported, this PR only add support to swift_repr = "struct"
For Vec support to actually be generated, the struct must be annotated with #[derive(Copy, Clone)]
- Vec support is also generated if the struct is annotated with only #[derive(Clone)], this is useful for structs with fields that cannot implement Copy (like String), however there is a performance overhead of using Clone rather than Copy, so it is advised to include #[derive(Copy)] as well.
- For Vec support to be generated, you MUST use the #[derive] macro, implementing the trait with impl Copy for Struct {} won't generate the Vec support code.

Here is an example of the Rust code:

#[swift_bridge::bridge]
mod ffi {
    #[swift_bridge(swift_repr = "struct")]
    #[derive(Copy, Clone)]
    struct SomeStruct {
        field: u8,
    }

    #[swift_bridge(swift_repr = "struct")]
    #[derive(Clone)] // This is supported but not recommended because of performance overhead of executing .clone() when getting elements from the Vec/Array
    struct AnotherStruct {
        field: String,
    }

    extern "Rust" {
        fn do_something(vec1: Vec<SomeStruct>, vec2: Vec<AnotherStruct>);
    }
}

pub fn do_something(vec1: Vec<SomeStruct>, vec2: Vec<AnotherStruct>) {
    /* do your thing */
}

And then on Swift you can:

let vec = RustVec<SomeStruct>()
vec.push(SomeStruct(field: 10))

let vec2 = RustVec<AnotherStruct>()
vec2.push(AnotherStruct(field: "cool".intoRustString()))

do_something(vec, vec2)

# Conflicts: # crates/swift-bridge-ir/src/codegen/generate_rust_tokens/shared_struct.rs

…ft-bridge into feature/shared-struct-derive

# Conflicts: # crates/swift-bridge-ir/src/codegen/generate_rust_tokens/shared_struct.rs

rkreutz · 2023-03-16T22:56:54Z

crates/swift-bridge-ir/src/codegen/generate_rust_tokens/vec/vec_of_transparent_struct.rs

+        let vec_map = if shared_struct.derives.copy {
+            quote! { *v }
+        } else {
+            quote! { v.clone() }
+        };


Notice that if the struct has the Copy derive we'll opt for using a copy rather than a clone. This whole block is guarded by an if statement checking if can_generate_vec_of_transparent_struct_functions, which at the moment will only return true if it is a swift_repr = "struct" with a #[derive(Copy, Clone)] or #[derive(Clone)].

rkreutz · 2023-03-17T22:19:20Z

@chinedufn this should be ready to be reviewed once you have the time. Cheers

chinedufn · 2023-03-17T23:23:35Z

Awesome! I'll review this weekend.
Thanks a lot for the detailed PR body. I found it to be very helpful.

Vec support is also generated if the struct is annotated with only #[derive(Clone)]

I don't think that we can land this part. swift-bridge is designed for high performance, zero or near-zero overhead bridging library.
Users need to trust that swift-bridge will never be the root cause of a surprising performance issue in their systems.

This means that implicit cloning is unacceptable.

I think that we should land the Copy vec support, ditch the Clone support and implement Vec<NonCopyStruct> after swift_repr = "class" support lands.

rkreutz · 2023-03-18T01:02:43Z

Do you think we could put this kind of behavior behind a flag on the crate, so people can explicitly opt for the degraded performance in favor of supporting String fields?

chinedufn · 2023-03-18T02:55:10Z

What's your motivation?

I'd rather just implement swift_repr = "class" and have a performant solution to this problem, but I'm curious about your thoughts / motives here?

rkreutz · 2023-03-18T23:46:41Z

Hey,

yeah, so my reasoning is that on most cases structs would contain Strings or other fields that can't implement Copy, so this change wouldn't be of much use. I understand that we could wait until swift_repr = "class" support is implemented, but I don't see that happening anytime soon tbh (at least I have no clue how to transform the pointers between Rust repr and Swift repr, so I don't think there is a solution any time soon), so Vec support will effectively take much more time to be added here.

About using pointers to pass the values around and not needing to use Copy/Clone, I guess whatever implementation we come up with will be compatible with both struct and class Swift repr.

I was thinking about how to do it, the main issue I've stumbled is that we pass a pointer to the Rust repr to Swift (on current Vec support that is) so Swift doesn't have a way to transform that Pointer into it's own repr, so we would have to start using the FFI repr pointer instead, which both Swift and Rust can understand, but that would mean that we would need to change somehow fn arguments and return values to be of type *const Vec<ffi_repr> rather than *const Vec<rust_repr>, which I have no clue how to do...

I might be completely wrong on how to approach this as well. But yeah that was pretty much why I thought would be a good idea to enable the use of Clone...

chinedufn · 2023-03-20T03:11:10Z

Gotcha, thanks for explaining. Yeah, agreed that Copy Vec support isn't super useful.

I'll take some time on Tuesday to think through swift_repr = "class" and write down a potential design.
I might be able to whip up an implementation of the design.

I had some ideas pop into my head earlier today. It might actually be fairly straightforwards.. but I have to sketch it out to be sure..

If it ends up being easy we can

Add swift_repr = "class"
Try to land Vec support for classes.

I would say that no matter what we probably won't land clone based Vec support, even behind a feature flag.
I get that it's a quick fix, but it strays too far from our high performance goals.

Happy to land this PR with Copy swift_repr = "struct" support.

Otherwise I should have time to design how swift_repr = "class" can work on Tuesday.

rkreutz · 2023-03-20T18:25:27Z

Hey, I think it will be better to just wait until we land the class repr Vec support then we can do the same for struct. Cheers

chinedufn · 2023-03-22T01:12:57Z

Sounds good. Feel free to subscribe to the swift_repr = "class" issue.

Just wrote down a design ~~that I think should work~~ idea but I think it needs more iteration... #196 (comment)

# Conflicts: # crates/swift-bridge-ir/src/codegen/generate_c_header.rs

rkreutz · 2023-04-09T22:39:43Z

hey @chinedufn I've added a new feature flag to the package here (compatibility) which would prioritise compatibility over performance. If the feature is enabled then struct deriving Clone and not Copy would use .clone() on the vec support code; if the feature is disabled (which is the default) structs that don't derive Copy wouldn't have the vec support code.

If you still don't think this is worth it feel free to close this PR, I'll be using my fork for the time being since I needed this functionality for a project I'm working on.

Cheers

chinedufn · 2023-04-10T17:16:15Z

If you still don't think this is worth it feel free to close this PR, I'll be using my fork for the time being since I needed this functionality for a project I'm working on.

I don't think that a feature flag should land but I'll still keep it open since once class based structs are solved your code will have us most of the way there.

My apologies if this is at all frustrating for you.
Thanks for your work and I'm very glad to hear that you have a solution in the meantime!

rkreutz · 2023-04-10T17:40:42Z

No problem at all, will leave it open then. Cheers

wooden-worm · 2024-01-09T08:35:59Z

I really need this feature, how far are we from landing class based structs and unblock this @chinedufn?

rkreutz and others added 20 commits March 11, 2023 00:09

Adding Vec support for transparent structs

5155eed

Adding derive() macro support to SharedStructs

9fb3327

Merge branch 'feature/shared-struct-derive'

a97ea5f

# Conflicts: # crates/swift-bridge-ir/src/codegen/generate_rust_tokens/shared_struct.rs

Checking for derives before generating vec support code

1a6af92

Code review

ead0af9

Merge branch 'feature/shared-struct-derive'

19babd5

Adjusting code to use StructDerives

91eaa86

Adding integration test

c4c916f

Merge branch 'feature/shared-struct-derive'

a9f87e7

Adding extra check

7eb667e

add missing mod

f7d2b7e

Merge branch 'feature/shared-struct-derive'

4a03b54

Add doc comments to codegen tests

a789e6b

Adding extra tests

f531c29

cargo fmt

b1e043f

Merge branch 'feature/shared-struct-derive' of github.com:rkreutz/swi…

7b61648

…ft-bridge into feature/shared-struct-derive

Merge branch 'feature/shared-struct-derive'

3031db7

cargo fmt

e401f32

Adjusting test comments

b08764e

Merge branch 'master' into feature/struct-vec-support

568233c

# Conflicts: # crates/swift-bridge-ir/src/codegen/generate_rust_tokens/shared_struct.rs

rkreutz mentioned this pull request Mar 16, 2023

Adding Vec support for transparent structs #193

Closed

rkreutz commented Mar 16, 2023

View reviewed changes

rkreutz mentioned this pull request Mar 23, 2023

Support swift_repr = "class" to use a Swift class for transparent structs #196

Open

rkreutz added 2 commits April 9, 2023 03:44

Merge branch 'master' into feature/struct-vec-support

9011fb3

# Conflicts: # crates/swift-bridge-ir/src/codegen/generate_c_header.rs

Adding compatibility feature flag

d7e3aeb

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Adding Vec support for transparent structs #199

Adding Vec support for transparent structs #199

rkreutz commented Mar 16, 2023

rkreutz Mar 16, 2023

rkreutz commented Mar 17, 2023

chinedufn commented Mar 17, 2023

rkreutz commented Mar 18, 2023

chinedufn commented Mar 18, 2023

rkreutz commented Mar 18, 2023

chinedufn commented Mar 20, 2023

rkreutz commented Mar 20, 2023

chinedufn commented Mar 22, 2023 •

edited

Loading

rkreutz commented Apr 9, 2023

chinedufn commented Apr 10, 2023

rkreutz commented Apr 10, 2023

wooden-worm commented Jan 9, 2024

Adding Vec support for transparent structs #199

Are you sure you want to change the base?

Adding Vec support for transparent structs #199

Conversation

rkreutz commented Mar 16, 2023

rkreutz Mar 16, 2023

Choose a reason for hiding this comment

rkreutz commented Mar 17, 2023

chinedufn commented Mar 17, 2023

rkreutz commented Mar 18, 2023

chinedufn commented Mar 18, 2023

rkreutz commented Mar 18, 2023

chinedufn commented Mar 20, 2023

rkreutz commented Mar 20, 2023

chinedufn commented Mar 22, 2023 • edited Loading

rkreutz commented Apr 9, 2023

chinedufn commented Apr 10, 2023

rkreutz commented Apr 10, 2023

wooden-worm commented Jan 9, 2024

chinedufn commented Mar 22, 2023 •

edited

Loading